Implement AuthenticationTagMismatchException exception by vcsjones · Pull Request #75160 · dotnet/runtime

vcsjones · 2022-09-06T19:45:00Z

This introduces a new AuthenticationTagMismatchException for AEAD ciphers to throw during decryption when the authentication tag produces a mismatch.

Previously for Android, we were assuming any failure meant "auth tag mismatch". This changes the native shim to properly catch and handle the Android AEADBadTagException.

Closes #67082

ghost · 2022-09-06T19:45:07Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2022-09-06T19:45:11Z

Tagging subscribers to this area: @dotnet/area-system-security, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

This introduces a new AuthenticationTagMismatchException for AEAD ciphers to throw during decryption when the authentication tag produces a mismatch.

Previously for Android, we were assuming anything mean "auth tag mismatch". This changes the native shim to properly catch and handle the Android AEADBadTagException.

Author:	vcsjones
Assignees:	-
Labels:	`area-System.Security`, `new-api-needs-documentation`
Milestone:	-

vcsjones · 2022-09-06T19:45:21Z

Draft to get a run through CI. Android is very unhappy in my local dev.

vcsjones · 2022-09-06T19:55:32Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-09-06T19:55:52Z

Azure Pipelines successfully started running 1 pipeline(s).

vcsjones · 2022-09-07T02:05:43Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-09-07T02:06:03Z

Azure Pipelines successfully started running 1 pipeline(s).

vcsjones · 2022-09-07T14:53:49Z

The x86 and x64 Android legs passed. The ARM64 seemed to crash. I also cannot get the ARM64 tests to run locally, but that's true even on main, so I don't think I introduced any incorrect behavior.

Marking as ready for review, then.

bartonjs · 2022-09-19T16:33:05Z

@jkoritzinsky can you weigh in on the JNI parts?

vcsjones · 2022-09-28T23:44:12Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-09-28T23:44:32Z

Azure Pipelines successfully started running 1 pipeline(s).

vcsjones · 2022-09-29T03:55:43Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-09-29T03:56:16Z

Azure Pipelines successfully started running 1 pipeline(s).

vcsjones · 2022-09-29T13:35:23Z

@bartonjs @jkoritzinsky I am having a hard time parsing the results of the Android test runs. I don't see anything in the test results that look like a failure as a result from this change. Can either of you help me confirm that and this is okay to merge?

filipnavara · 2022-09-29T13:50:52Z

It is related:

09-29 06:08:20.250  3012  3012 I DOTNET  : JNI_OnLoad: JNI_OnLoad in pal_jni.c
09-29 06:08:20.251  3012  3012 F DOTNET  : GetClassGRef: class javax/crypto/spec/AEADBadTagException was not found
--------- beginning of crash
09-29 06:08:20.251  3012  3012 F libc    : Fatal signal 6 (SIGABRT), code -1 (SI_QUEUE) in tid 3012 (rimitives.Tests), pid 3012 (rimitives.Tests)

Taken from one of the MonoRunner logs here

…l_jni.c Co-authored-by: Filip Navara <filip.navara@gmail.com>

vcsjones · 2022-09-29T13:57:30Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-09-29T13:57:43Z

Azure Pipelines successfully started running 1 pipeline(s).

vcsjones · 2022-09-29T17:31:08Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-09-29T17:31:22Z

Azure Pipelines successfully started running 1 pipeline(s).

vcsjones · 2022-09-30T02:05:20Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-09-30T02:05:41Z

Azure Pipelines successfully started running 1 pipeline(s).

vcsjones added 2 commits September 6, 2022 14:22

Throw AuthenticationTagMismatchException for AEAD tag mismatches

f431d17

Properly handle Android

cebac82

ghost added area-System.Security new-api-needs-documentation labels Sep 6, 2022

ghost assigned vcsjones Sep 6, 2022

vcsjones commented Sep 6, 2022

View reviewed changes

Comment thread src/native/libs/System.Security.Cryptography.Native.Android/pal_cipher.c Outdated

vcsjones changed the title ~~Auth tag mismatch~~ Implement AuthenticationTagMismatchException exception Sep 6, 2022

vcsjones added 2 commits September 6, 2022 21:38

Fix test

52359e9

Restart CI

3f88ad9

vcsjones marked this pull request as ready for review September 7, 2022 14:53

vcsjones commented Sep 7, 2022

View reviewed changes

Comment thread ...ies/Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.Cipher.cs

bartonjs reviewed Sep 7, 2022

View reviewed changes

Comment thread src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/AesCcm.Android.cs

bartonjs requested a review from jkoritzinsky September 7, 2022 16:15

vcsjones mentioned this pull request Sep 28, 2022

Implement ChaCha20Poly1305 with CryptoKit on macOS #76317

Merged

vcsjones added 2 commits September 28, 2022 15:17

Merge remote-tracking branch 'ms/main' into auth-tag-mismatch

a714127

Keep AEADBadTagException as a gref

700da4a

jkoritzinsky approved these changes Sep 28, 2022

View reviewed changes

bartonjs approved these changes Sep 28, 2022

View reviewed changes

filipnavara reviewed Sep 29, 2022

View reviewed changes

Comment thread src/native/libs/System.Security.Cryptography.Native.Android/pal_jni.c Outdated

Update src/native/libs/System.Security.Cryptography.Native.Android/pa…

8a04abb

…l_jni.c Co-authored-by: Filip Navara <filip.navara@gmail.com>

runfoapp Bot mentioned this pull request Sep 29, 2022

system.text.regularexpressions.tests work item #72834

Open

Do not pass null pointer for empty destinations

4f1c3f8

jkoritzinsky reviewed Sep 29, 2022

View reviewed changes

Comment thread ...ies/Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.Cipher.cs

Add comment explaining null handling

3791997

bartonjs approved these changes Sep 29, 2022

View reviewed changes

vcsjones merged commit 60b40c3 into dotnet:main Sep 30, 2022

vcsjones deleted the auth-tag-mismatch branch September 30, 2022 12:57

MattGal mentioned this pull request Sep 30, 2022

Production - [Alerting] Android emulator failure rate alert dotnet/arcade#11052

Closed

runfoapp Bot mentioned this pull request Sep 30, 2022

Compiler crashes when failing to release Mutex #53420

Closed

ghost locked as resolved and limited conversation to collaborators Oct 30, 2022

Uh oh!

Conversation

vcsjones commented Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Sep 6, 2022

Uh oh!

ghost commented Sep 6, 2022

Uh oh!

vcsjones commented Sep 6, 2022

Uh oh!

Uh oh!

vcsjones commented Sep 6, 2022

Uh oh!

azure-pipelines Bot commented Sep 6, 2022

Uh oh!

vcsjones commented Sep 7, 2022

Uh oh!

azure-pipelines Bot commented Sep 7, 2022

Uh oh!

vcsjones commented Sep 7, 2022

Uh oh!

Uh oh!

Uh oh!

bartonjs commented Sep 19, 2022

Uh oh!

vcsjones commented Sep 28, 2022

Uh oh!

azure-pipelines Bot commented Sep 28, 2022

Uh oh!

vcsjones commented Sep 29, 2022

Uh oh!

azure-pipelines Bot commented Sep 29, 2022

Uh oh!

vcsjones commented Sep 29, 2022

Uh oh!

filipnavara commented Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

vcsjones commented Sep 29, 2022

Uh oh!

azure-pipelines Bot commented Sep 29, 2022

Uh oh!

vcsjones commented Sep 29, 2022

Uh oh!

azure-pipelines Bot commented Sep 29, 2022

Uh oh!

Uh oh!

vcsjones commented Sep 30, 2022

Uh oh!

azure-pipelines Bot commented Sep 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vcsjones commented Sep 6, 2022 •

edited

Loading

filipnavara commented Sep 29, 2022 •

edited

Loading